Make GraphicsResource inherit from Buffer#1701
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
Auto-sync is disabled for ready for review pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
/ok to test |
|
|
/ok to test |
| return | ||
| if self._mapped: | ||
| # Best-effort unmap before unregister | ||
| # Best-effort unmap before unregister (use stream 0 as fallback) |
There was a problem hiding this comment.
Potential stream-ordering bug here: map()/unmap() now require an explicit stream, but close() always unmaps on stream 0 and ignores both _map_stream and the optional close(stream=...) arg.
If a resource was mapped on a non-default stream and then closed while mapped, unmap can be issued on the wrong stream, which may break ordering guarantees with in-flight work. Could we unmap using _map_stream (or the passed stream, with a clear fallback policy) instead of hard-coding stream 0?
| self._map_stream = None | ||
|
|
||
| def __enter__(self): | ||
| return self |
There was a problem hiding this comment.
__enter__ currently returns self even when the resource is not mapped. That allows patterns like with GraphicsResource.from_gl_buffer(vbo) as buf: (without stream=), where buf.handle/buf.size are not valid.
Could we either (a) raise in __enter__ when not self._mapped, or (b) require/perform mapping for the context-manager path to avoid this silent footgun?
cpcloud
left a comment
There was a problem hiding this comment.
Thanks for the refactor here — the GraphicsResource(Buffer) direction looks good overall.
Requesting changes for two behavior issues called out inline:
close()currently unmaps on stream 0 even when mapping happened on a non-default stream, which can break stream-ordering guarantees.__enter__returnsselfeven when unmapped, allowing context-manager usage wherehandle/sizeare invalid.
Please address these and I’m happy to re-review.
Summary
GraphicsResourceto inherit fromBufferinstead of wrapping a separateBufferobject, eliminating the_MappedBufferContextintermediary classmap()now populates theGraphicsResourceitself with device pointer/size (since it IS-ABuffer) and returnsselfstreama required parameter formap()andunmap()(removes implicit default stream usage)streamparameter tofrom_gl_buffer()for a convenient register-and-map-in-one-step patternhandleproperty toresource_handleto avoid conflicting withBuffer.handle(which now exposes the mapped device pointer)Motivation
Review feedback requested that
GraphicsResourcebe aBufferdirectly rather than producing a separateBufferviamap(). This simplifies the API surface: a mappedGraphicsResourcecan be passed directly anywhere aBufferis accepted (e.g.,StridedMemoryView.from_buffer()), without needing a wrapper context manager.Key changes
GraphicsResource(Buffer)— inherits fromBuffer;handleandsizeare valid while mapped_MappedBufferContext— no longer needed sinceGraphicsResourceitself is the buffer__enter__/__exit__— moved ontoGraphicsResourcedirectly;__exit__auto-unmaps using the stream frommap()streamrequired —map(stream=)andunmap(stream=)no longer default to stream 0from_gl_buffer(stream=)— optional stream param to register + map in one call, enablingwith GraphicsResource.from_gl_buffer(vbo, stream=s) as buf:patternclose(stream=None)— acceptsstreamkwarg for compatibility withBuffer.close()resource_handle— renamed fromhandleto avoid shadowingBuffer.handle🤖 Generated with Claude Code